Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wang frenkel #1970

Open
wants to merge 29 commits into
base: trunk-minor
Choose a base branch
from
Open

Wang frenkel #1970

wants to merge 29 commits into from
+460 −3

Conversation

MartinGirard
Copy link
Contributor

@MartinGirard MartinGirard commented Dec 15, 2024 •
edited
Loading

Description

This draft PR would add the Wang-Frenkel potential to hoomd (equation (2) in Wang & Frenkel, https://pubs.rsc.org/en/content/articlelanding/2020/cp/c9cp05445f).

Motivation and context

This is largely motivated by coarse-grained IDP models, specifically by MPIPI (https://www.nature.com/articles/s43588-021-00155-3), which uses this type of potential.

How has this been tested?

This is work in progress, unit tests will be added to hoomd. At the time of writing this draft PR, I would want some feedback on the implementation. Specifically, in eq (2) of Wang et al, the cutoff radius appears in the potential. I have chosen to change "r_c" there by an adjustable parameter.

As the potential requires calculation of arbitrary powers, which are integers, this has required addition of pow(float, int) in hoomdmath.

Checklist:

  • I have reviewed the Contributor Guidelines.
  • I agree with the terms of the HOOMD-blue Contributor Agreement.
  • My name is on the list of contributors (sphinx-doc/credits.rst) in the pull request source branch.
  • I have summarized these changes in CHANGELOG.rst following the established format.

@MartinGirard MartinGirard marked this pull request as ready for review December 19, 2024 13:28
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale There has been no activity on this for some time. label Jan 8, 2025
@joaander joaander removed the stale There has been no activity on this for some time. label Jan 9, 2025
@joaander
Copy link
Member

Thanks. I'm busy right now but will review this when I get a chance. I would be helpful if you could address the failing CI checks.

@MartinGirard
Copy link
Contributor Author

Thanks. I'm busy right now but will review this when I get a chance. I would be helpful if you could address the failing CI checks.

Failing test is the pre-commit.ci.

joaander
joaander requested changes Jan 20, 2025
View reviewed changes
Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Regarding pre-commit: Yes -- you need to run pre-commit, commit the automatic changes, and make any manual adjustments needed to satisfy all the lints. See https://hoomd-blue.readthedocs.io/en/v5.0.1/style.html

Overall, this PR is very clean and complete. Aside from pre-commit, I have a few questions and requests around the new pow math functions. I will be happy to merge after these are addressed and the needed unit test is added.

hoomd/HOOMDMath.h Outdated Show resolved Hide resolved
hoomd/HOOMDMath.h Outdated Show resolved Hide resolved
hoomd/md/EvaluatorPairWangFrenkel.h Outdated Show resolved Hide resolved
hoomd/md/pair/pair.py Outdated Show resolved Hide resolved
@MartinGirard MartinGirard requested a review from joaander January 24, 2025 20:52
@MartinGirard
Copy link
Contributor Author

Should be more or less set now. There is a last detail, if users input negative mu or nu, the cast to uint in the evaluator fails (the exponent should always be positive for this potential to make sense anyway). The user receives a cryptic message:

RuntimeError: Unable to cast Python instance of type <class 'int'> to C++ type '?' (#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)

Python doesn't have built-in types for unsigned integers. I can check this on the C++ side, but that yields lengthy messages. Do we have a neater way on the python side?

@joaander
Copy link
Member

Do we have a neater way on the python side?

Yes, use a validator. We have one for positive reals, but not positive ints so you will need to write one:

hoomd-blue/hoomd/data/typeconverter.py

Lines 87 to 95 in 769ec59

def positive_real(number):
"""Ensure that a value is positive."""
try:
float_number = float(number)
except Exception as err:
raise TypeConversionError(f"{number} not convertible to float.") from err
if float_number <= 0:
raise TypeConversionError("Expected a number greater than zero.")
return float_number

Here is an example of how it gets used:

hoomd-blue/hoomd/md/pair/pair.py

Lines 328 to 332 in 769ec59

params = TypeParameter(
"params",
"particle_types",
TypeParameterDict(epsilon=float, sigma=positive_real, len_keys=2),
)

Using these will give a somewhat better formatted message indicating which parameter keys caused the error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joaander joaander Awaiting requested review from joaander

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MartinGirard @joaander